-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RSDK-1718 - Add GetInternalState to SLAM RDK #1776
Conversation
I don't understand why we would want this GRPC endpoint to have the side effect of saving the internal state to a file. Will that responsibility still live in RDK after slam modularization? |
t.Helper() | ||
|
||
position, err := svc.Position(context.Background(), "test", map[string]interface{}{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially it was set up to test the various endpoints in one fucntion, however these endpoints and their outputs do not relate to eachother. With the addition of get internal state and the fact that it may not be called every run. I choose to make these checks independent so any combination could be called.
I also choose to increase the robustness of testing by doing an inBetween check for the Position instead of just logging he output.
Happy to discuss either of these points as they are improvements rather than requirements for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you run the test several times to determine reasonable values for the position and orientation tolerance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should we make this change to the orbslam integration tests as well? Or expect to do that when we add GetInternalState to orbslam integration tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re @tessavitabile 's first point: when I ran tests on my mac, they consistently fail due to an actual value being out of range:
go test -run TestCartographerIntegration2D -v
...
logger.go:130: 2023-01-25T13:42:57.684-0500 ERROR process.slam_cartographer_carto_grpc_server pexec/managed_process.go:214 output {"name": "StdErr", "data": "I20230125 13:42:57.684520 377790 callbacks.cc:125] 8 1.154142e-01 2.07e-07 8.74e-03 1.08e-04 2.90e-01 7.00e+03 1 1.09e-04 1.23e-03"}
logger.go:130: 2023-01-25T13:42:57.684-0500 ERROR process.slam_cartographer_carto_grpc_server pexec/managed_process.go:214 output {"name": "StdErr", "data": "I20230125 13:42:57.684589 377790 trust_region_minimizer.cc:743] Terminating: Function tolerance reached. |cost_change|/cost: 9.051883e-07 <= 1.000000e-06"}
logger.go:130: 2023-01-25T13:42:57.684-0500 ERROR process.slam_cartographer_carto_grpc_server pexec/managed_process.go:214 output {"name": "StdErr", "data": "I20230125 13:42:57.684779 377790 slam_service.cc:721] Passed sensor data to SLAM /var/folders/y0/ff4chsnj1z19b6ps4wslbhyh0000gn/T/668702867/data/cartographer_int_lidar_data_2023-01-25T18:42:57.2978Z.pcd"}
logger.go:130: 2023-01-25T13:42:57.684-0500 ERROR process.slam_cartographer_carto_grpc_server pexec/managed_process.go:214 output {"name": "StdErr", "data": "I20230125 13:42:57.684928 377790 slam_service.cc:575] No new files found"}
cartographer_int_test.go:130: Expected '0.009272603568955304' to be between '0' and '0.008' (but it wasn't)!
--- FAIL: TestCartographerIntegration2D (4.73s)
FAIL
exit status 1
FAIL go.viam.com/rdk/services/slam/builtin 6.416s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We think this may be an issue where hardware is processing and save the artifact images a different speeds, leading to different timestamps and altering the calculation. I'll be investigating ways to mitigate that but we can't standardize the filename timestamps because that would defeat the purpose of the end to end test. At the very least we'll make sure tolerance values are sufficient (I've run it on my laptop's docker 20+ times with no failures)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I would like to make sure this is stable, both locally and in CI, before merging this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still fails on my mac:
go test -run TestCartographerIntegration2D -v
logger.go:130: 2023-01-30T12:17:29.681-0500 ERROR process.slam_cartographer_carto_grpc_server pexec/managed_process.go:218 output {"name": "StdErr", "data": "I20230130 12:17:29.681169 3117346 slam_service.cc:616] Passed sensor data to SLAM /var/folders/y0/ff4chsnj1z19b6ps4wslbhyh0000gn/T/2425438821/data/cartographer_int_lidar_data_2023-01-30T17:17:29.4168Z.pcd"}
logger.go:130: 2023-01-30T12:17:29.681-0500 ERROR process.slam_cartographer_carto_grpc_server pexec/managed_process.go:218 output {"name": "StdErr", "data": "I20230130 12:17:29.681468 3117346 slam_service.cc:470] No new files found"}
cartographer_int_test.go:130: Expected '0.009183805047824956' to be between '0' and '0.008' (but it wasn't)!
--- FAIL: TestCartographerIntegration2D (7.20s)
FAIL
exit status 1
FAIL go.viam.com/rdk/services/slam/builtin 8.997s
actualMIME, _, pointcloud, err := svc.GetMap(context.Background(), "test", "pointcloud/pcd", nil, false, map[string]interface{}{}) | ||
test.That(t, err, test.ShouldBeNil) | ||
test.That(t, actualMIME, test.ShouldResemble, "pointcloud/pcd") | ||
test.That(t, pointcloud.Size(), test.ShouldBeGreaterThanOrEqualTo, 100) | ||
} | ||
|
||
// Checks the cartographer position. | ||
func testCartographerPosition(t *testing.T, svc slam.Service) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above
@@ -5,15 +5,18 @@ import ( | |||
"io/ioutil" | |||
"os" | |||
"os/exec" | |||
"path/filepath" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we changing integration tests if the PR description says that integration tests will come later? #1776 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked offline but testing was needed to validate and it wasn't much work (replicating work done during the GetInternalState for carto) so I chose to include it here
@@ -162,6 +175,21 @@ func TestClientWorkingService(t *testing.T) { | |||
test.That(t, err, test.ShouldBeNil) | |||
test.That(t, p.Parent(), test.ShouldEqual, pSucc.Parent()) | |||
test.That(t, extraOptions, test.ShouldResemble, extra) | |||
|
|||
// test get map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added missing test for GetMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
switch { | ||
case mode == "mono" && actionMode == "mapping": | ||
expectedPos = r3.Vector{X: 0.020, Y: -0.032, Z: -0.053} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defined expected position and orientation here as opposed to passing them into the function as it centralizes any changes to the expected values to one location.
Let me know if you'd prefer we handle this a different way.
@@ -1117,6 +1117,10 @@ func TestGetMapAndPosition(t *testing.T) { | |||
test.That(t, pc, test.ShouldBeNil) | |||
test.That(t, fmt.Sprint(err), test.ShouldContainSubstring, "error getting SLAM map") | |||
|
|||
internalState, err := svc.GetInternalState(context.Background(), "hi") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: most testing of endpoints has to be done in integration tests. This just confirms the failure/error statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an issue in the test changes
|
||
actualOri := position.Pose().Orientation().AxisAngles() | ||
t.Logf("Position orientation: RX: %v, RY: %v, RZ: %v, Theta: %v", actualOri.RX, actualOri.RY, actualOri.RZ, actualOri.Theta) | ||
test.That(t, actualOri.RX, test.ShouldBeBetween, expectedOri.OX-toleranceOri, expectedOri.OX+toleranceOri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we comparing RX (axis angle) to OX (orientation vector) here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call out, I'll make sure these are of the same type. Will most likely go with Axis angle due to readability and as that was what we were using before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks!
|
||
actualOri := position.Pose().Orientation().AxisAngles() | ||
t.Logf("Position orientation: RX: %v, RY: %v, RZ: %v, Theta: %v", actualOri.RX, actualOri.RY, actualOri.RZ, actualOri.Theta) | ||
test.That(t, actualOri.RX, test.ShouldBeBetween, expectedOri.OX-toleranceOri, expectedOri.OX+toleranceOri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, it seems incorrect that we're comparing axis angle representation to orientation vector representation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM mod making sure we're comparing orientations of the same type
t.Helper() | ||
|
||
position, err := svc.Position(context.Background(), "test", map[string]interface{}{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still fails on my mac:
go test -run TestCartographerIntegration2D -v
logger.go:130: 2023-01-30T12:17:29.681-0500 ERROR process.slam_cartographer_carto_grpc_server pexec/managed_process.go:218 output {"name": "StdErr", "data": "I20230130 12:17:29.681169 3117346 slam_service.cc:616] Passed sensor data to SLAM /var/folders/y0/ff4chsnj1z19b6ps4wslbhyh0000gn/T/2425438821/data/cartographer_int_lidar_data_2023-01-30T17:17:29.4168Z.pcd"}
logger.go:130: 2023-01-30T12:17:29.681-0500 ERROR process.slam_cartographer_carto_grpc_server pexec/managed_process.go:218 output {"name": "StdErr", "data": "I20230130 12:17:29.681468 3117346 slam_service.cc:470] No new files found"}
cartographer_int_test.go:130: Expected '0.009183805047824956' to be between '0' and '0.008' (but it wasn't)!
--- FAIL: TestCartographerIntegration2D (7.20s)
FAIL
exit status 1
FAIL go.viam.com/rdk/services/slam/builtin 8.997s
|
This PR ands the
GetInternalState
functionality to the SLAM service in RDK. Similarly to thePosition
andGetMap
, this function acts as a middleman, forwarding requests from RDK to the SLAM algorithm client and returning the response.Currently
GetInternalState
returns a byte slice and an error.Unit testing has been added as part of this PR for the client and server. Integration testing for cartographer has also been incorporated into this PR as it needed to be tested. Future integration testing will be necessary for orbslamv3.
JIRA Ticket: RSDK-1718
Associated JIRA Ticket: RSDK-1056
Associated PRs: PR #137 [carto] Implement GetInternalState
Note: Testing will not pass until the associated PR is merged.